-
-
Notifications
You must be signed in to change notification settings - Fork 80
pybricks.tools: Refactor awaitable type. #385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dlech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff.
I would feel even better about it if we could add a bunch of unit tests (in Python) that prove this works correctly in all sorts of use cases.
| /** | ||
| * Cancels the iterable so it will stop awaiting. | ||
| * | ||
| * This will not call close(). Safe to call even if iter is NULL or if it is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this doesn't call close(), it sounds more like an abort than a cancel. In computer science, abort means stop it without any additional code executing while cancel means ask it nicely to stop what it is doing and be sure to clean up after yourself and leave things in a consistent state.
For proper cancellation, it is important the async functions can make sure they are not in the middle of an operation that can't be canceled. So it would be problematic to use this with those async functions, e.g. some of the Bluetooth stuff.
I think we should at least document these caveats here and say that this is really only meant to be used in a highly managed way where something else is scheduled to immediately take the place of the aborted iterator. In other words, this only works in the context of pb_type_Task that ensures that close() isn't called on an aborted task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, this only works in the context of pb_type_Task that ensures that close() isn't called on an aborted task.
These are two different things.
async def main1():
await motor.run_angle(500, very_large_angle)
print("we will never see this because the task got closed")
async def main2():
await wait(1000)
run_task(multitask(main1(), main2(), race=True))Wait wins the race so the task wraps up and close ensures the motor stops.
async def main1():
await motor.run_angle(500, very_large_angle)
print("task not interrupted, just the motor is done here. we do see this")
async def main2():
await wait(1000)
await motor.run_angle(-500, very_large_angle)
run_task(multitask(main1(), main2()))Here, the motor starts off moving for a long time. But after one second, the same motor is started in reverse from elsewhere. This is safe and fine (new motor command always wins), but we do want the original awaitable to know to stop waiting, which is what pb_type_async_schedule_cancel (or abort) does.
So it schedules the awaitable to cancel the next time it is iterated, so it isn't quite aborted right away. I don't think there is a way to do that, since the caller main1 needs to be the one exhausting it.
I'll include variants of these in the unit tests.
Since this doesn't call close(), it sounds more like an abort than a cancel. In computer science, abort means stop it without any additional code executing while cancel means ask it nicely to stop what it is doing and be sure to clean up after yourself and leave things in a consistent state.
By this definition it is somewhere in between, but I don't mind calling it pb_type_async_schedule_abort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the right term is pb_type_async_schedule_stop_iteration since that is what it does.
Can be used to await protothreads, ultimately simplifying most of our awaitable pbio operations.
It turns out that MP_OBJ_STOP_ITERATION is the same as MP_OBJ_NULL, so it can't be used as an intermediate stage. We'll use MP_OBJ_SENTINEL instead and make yield-once functions work another way.
No more linked lists to keep track of awaitables. This fixes programs running out of memory if the user forgets await. Still allow avoiding allocation in basic scripts using a fixed number of statically allocated waits.
This new awaitable is introduced so we can eventually drop queued tasks and safely await protothreads. For now, just keep the pbio tasks compiling.
Now that we can have protothreads as the iterable function, we can trivially do these in C instead of importing Python code from C. Now they can correctly be cancelled, too. A new text animation will take over and stop any ongoing ones.
Also generalize return map to pass in the sensor object. This allows the return map to make use of the sensor state.
Writing both the def and async def for each user method is cumbersome. This allows the user to make a read call and specify how to map the bytes to a return value. This method may then be called as async or sync without further wrappers.
Also flush UART when opening, in case program stopped during prior transmission.
All modules have been upgraded, so we can drop this.
This would shedule cancellation even when already completed. This flagged the awaitable not ready for recycling, so would cause allocation when this isn't needed.
All comments incorporated. Very helpful, thanks!
Good idea. Incoming. |
Almost everywhere we use pb_type_async_wait_or_await we stop the ongoing iteration first, so include that functionality as a bool. It is still also available separately for methods that want to stop ongoing awaitables without spawning another one. Also rename pb_type_async_schedule_cancel to pb_type_async_schedule_stop_iteration since cancel isn't quite the right word and this makes it more explicit what it does.
|
I wonder if we can test allocation on the virtual hub? It allocates all the time since it isn't running the compiled |
|
What sort of allocation and testing do you mean? I'm not understanding the question. |
|
Having test cases to confirm when it does and does not allocate could be useful. async def example():
while True:
await motor.run_angle(SPEED, 90)
await wait(500)
await motor.run_angle(SPEED, -90)
await wait(500)For simple scripts like these, this new type ensures that this will not allocate (other than in the very first time it gets to the motor command). This is easy to confirm with |
That's weird. I would not expect it to. It uses the same runtime after all. |
This is a rewrite of
pb_type_awaitable. This type is responsible for making functions and methods across the Pybricks API async-compatible for use withawait.Adds support for awaiting
pbio/osprotothreadsThis will make it much simpler to await certain interactions with drivers. We'll use this for upcoming changes to the Bluetooth drivers in pybricks/support#2252
Simplified cancellation and reuse
The parent object (e.g. a
Motorinstanced) used to have a list object to keep track of associated awaitables.Depending on flags set when a new awaitable is made, it would cancel those before spawning another one. In practice, we only have the "most recent" awaitable that could still be in use, so now we only keep track of that one. We'll re-use it if it has already been fully exhausted and allocate another one otherwise. This also allows unused awaitables to be garbage collected rather than raise a
MemoryErrorif you made too many before awaiting them.Other awaitable option flags are also removed, because we ended up not using them. Instead, wherever we used
PB_TYPE_AWAITABLE_OPT_RAISE_ON_BUSY, it makes more sense to raise the error explicitly in the specific method. For example,I2CDevice.writewill raise right away rather than deferring this to the awaitable.Simplifications for some existing awaitable methods
Awaitable functions that involved more than one operation were very akward to write before, to the point that we ended up writing them in Python and calling them from C code. We don't need to do that anymore. Besides simplicity, this also keep things like cancellation simple.
For example, the awaitable function for
hub.speaker.play_notesis now as follows, which is pretty neat. Raising from these functions using MicroPython APIs is allowed, but the return type is stillpbio_error_tfor compatibility with internal functions.Raising from these functions using MicroPython APIs is allowed, but the return type is still
pbio_error_tfor compatibility with internal functions.Regressions
Bluetooth hasn't been revisited yet, so cancellation of the scan and connect function of remotes is not currently implemented yet.